Enforce canonical dispatch-repository safe-output key (deprecate underscore alias)#42150
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
dispatch_repository safe-output key (remove dashed alias)
🤖 PR Triage — §28342769269
Score breakdown: Impact 20 · Urgency 8 · Quality 12 Rationale: Tiny targeted fix removing undocumented dashed alias from
|
|
Great work on this one! 🎉 The
This PR looks ready for review. 🟢
|
|
@copilot align using dashes like other safe outputs |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot create a codemod to rename to the proper syntax |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🤖 PR Triage — §28357644191
Carried over — 7.9h old. Removes undocumented dashed alias for
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
This PR updates the safe-outputs key name for the repository-dispatch safe output, shifting the canonical key from dispatch_repository (underscore) to dispatch-repository (dash) while retaining underscore as a deprecated alias (plus a codemod to migrate).
Changes:
- Make
dispatch-repositorythe canonical runtime parse key (withdispatch_repositoryas a fallback alias). - Update schema + schema validation tests to define/accept
dispatch-repositoryand markdispatch_repositoryas a deprecated alias. - Add a
gh aw fixcodemod to rewritesafe-outputs.dispatch_repository→safe-outputs.dispatch-repository, and update docs/tests/warnings accordingly.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/dispatch_repository.go | Switches runtime parsing to prefer dispatch-repository (underscore fallback). |
| pkg/workflow/dispatch_repository_test.go | Updates unit tests to use dispatch-repository as primary and keep underscore as alias coverage. |
| pkg/workflow/dispatch_repository_experimental_warning_test.go | Updates expected experimental warning text to dispatch-repository. |
| pkg/workflow/compiler_validators.go | Updates emitted experimental warning string to dispatch-repository. |
| pkg/parser/schemas/main_workflow_schema.json | Makes dispatch-repository canonical in schema and adds dispatch_repository as a deprecated alias. |
| pkg/parser/schema_test.go | Updates schema path assertions to reference dispatch-repository. |
| pkg/parser/schema_location_test.go | Adds coverage that dispatch-repository is accepted by schema validation. |
| pkg/cli/fix_codemods.go | Registers the new dispatch-repository key migration codemod. |
| pkg/cli/fix_codemods_test.go | Updates expected codemod list/order to include the new codemod. |
| pkg/cli/codemod_safe_output_dispatch_repository_key.go | Implements migration from underscore key to dashed key in frontmatter text. |
| pkg/cli/codemod_safe_output_dispatch_repository_key_test.go | Adds unit tests validating the codemod behavior (rename, indentation/comments, no-ops). |
| docs/src/content/docs/reference/safe-outputs.md | Updates docs to describe/configure dispatch-repository and the warning string. |
| docs/src/content/docs/reference/glossary.md | Updates glossary entry and reference links to dispatch-repository. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
pkg/workflow/dispatch_repository.go:45
- The parser now treats
dispatch-repositoryas canonical, but the nearby debug log messages still refer todispatch_repository, which can make troubleshooting misleading. Update these log strings to match the canonical key name (and ideally keep the terminology consistent throughout the function).
}
configMap, ok := configData.(map[string]any)
if !ok {
dispatchRepositoryLog.Print("dispatch_repository value is not a map, skipping")
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Low
| // dispatch-repository is canonical; keep underscore form as a backward-compatible alias. | ||
| if configData, exists = outputMap["dispatch-repository"]; !exists { | ||
| if configData, exists = outputMap["dispatch_repository"]; !exists { | ||
| return nil | ||
| } |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
Skills-Based Review Summary 🧠Applied Verdict: REQUEST_CHANGES — two substantive issues plus minor suggestions. 🔴 Blocking issues (2)
🟡 Suggestions (4)
@copilot please address the review comments above.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /zoom-out, /tdd, /diagnose, and /improve-codebase-architecture — requesting changes primarily on the PR title/description accuracy and a missing precedence test.
📋 Key Themes & Highlights
Key Themes
- PR title/description contradict the implementation: The title says "Enforce canonical
dispatch_repository" (underscore) and the description includes an "after: underscore-only" snippet, but the implementation makes the dashed form canonical (dispatch-repositorychecked first) and deprecates underscore. The code is correct and consistent with all other safe-output keys (which all use dashes), but the misleading title will cause confusion for reviewers and future maintainers. - Missing precedence test: The parser now checks dashed before underscore, but no test exercises the case where both keys coexist in the same map. This is the core behaviour change and should be regression-tested.
- Silent deprecation: Users with
dispatch_repositoryin existing workflows get no compiler warning; they must know to rungh aw fixindependently.
Positive Highlights
- ✅ Codemod is well-structured: correct no-op guards, indentation/comment preservation, and four meaningful test cases
- ✅ Schema change correctly places the dashed form as the primary property and the underscore as a
$refalias — matches project conventions - ✅ Documentation and anchor links updated consistently throughout
- ✅ Experimental warning message updated in sync with the key rename
- ✅
TestParseDispatchRepositoryConfig_UnderscoreAliascorrectly documents the backward-compat contract
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 96.8 AIC · ⌖ 8.35 AIC · ⊞ 6.6K
Comment /matt to run again
| if configData, exists = outputMap["dispatch_repository"]; !exists { | ||
| if configData, exists = outputMap["dispatch-repository"]; !exists { | ||
| // dispatch-repository is canonical; keep underscore form as a backward-compatible alias. | ||
| if configData, exists = outputMap["dispatch-repository"]; !exists { |
There was a problem hiding this comment.
[/zoom-out] The implementation makes dispatch-repository (dashed) canonical, but the PR title says "Enforce canonical dispatch_repository safe-output key (remove dashed alias)" — the opposite direction. The code is correct (all other safe-output keys use dashes: dispatch-workflow, call-workflow, merge-pull-request), but the title and PR description "after: underscore-only" snippet are misleading and could confuse future maintainers.
💡 Suggested title
Consider retitling to something like:
Enforce canonical
dispatch-repositorysafe-output key (deprecate underscore alias)
And update the PR description code snippet to match the actual implementation, which still accepts both forms (dashed preferred).
@copilot please address this.
| config := compiler.parseDispatchRepositoryConfig(outputMap) | ||
| require.NotNil(t, config, "Config should be parsed from dash form") | ||
| require.NotNil(t, config, "Config should be parsed from underscore alias") | ||
| require.Len(t, config.Tools, 1, "Should have 1 tool") |
There was a problem hiding this comment.
[/tdd] The renamed test verifies the underscore alias still works, but the new precedence (dashed checked first) is not tested. When both dispatch-repository and dispatch_repository exist in the same map, the dashed value should win — but there is no test for this.
💡 Suggested test
func TestParseDispatchRepositoryConfig_DashPrecedenceOverUnderscore(t *testing.T) {
compiler := NewCompiler(WithVersion("1.0.0"))
outputMap := map[string]any{
"dispatch-repository": map[string]any{
"dash_tool": map[string]any{
"workflow": "dash.yml",
"event_type": "dash_event",
"repository": "github/canonical",
},
},
"dispatch_repository": map[string]any{
"underscore_tool": map[string]any{
"workflow": "underscore.yml",
"event_type": "underscore_event",
"repository": "github/alias",
},
},
}
config := compiler.parseDispatchRepositoryConfig(outputMap)
require.NotNil(t, config)
_, hasDashTool := config.Tools["dash_tool"]
assert.True(t, hasDashTool, "dashed form should take precedence")
_, hasUnderscoreTool := config.Tools["underscore_tool"]
assert.False(t, hasUnderscoreTool, "underscore form should be shadowed by dashed form")
}@copilot please address this.
| }, | ||
| }, | ||
| filePath: "/test/workflow.md", | ||
| wantErr: false, |
There was a problem hiding this comment.
[/zoom-out] The test correctly uses wantErr: false (the dashed key should be accepted), but the PR description says "Added parser validation coverage to ensure safe-outputs.dispatch-repository fails schema/location validation." — the opposite of what this test asserts. Either the test or the PR description is wrong; based on the schema change (dashed form is now canonical), the test is correct.
Please update the PR description to accurately reflect the intent of this test case.
@copilot please address this.
| if configData, exists = outputMap["dispatch-repository"]; !exists { | ||
| // dispatch-repository is canonical; keep underscore form as a backward-compatible alias. | ||
| if configData, exists = outputMap["dispatch-repository"]; !exists { | ||
| if configData, exists = outputMap["dispatch_repository"]; !exists { |
There was a problem hiding this comment.
[/diagnose] When the underscore alias is used as the fallback path (no dispatch-repository key found), the compiler silently accepts it with no deprecation signal to the user. Users who have not yet migrated their workflows will have no indication that they need to run gh aw fix — unless they happen to check the changelog.
💡 Suggested improvement
Consider emitting a deprecation warning when the underscore fallback is taken:
if configData, exists = outputMap["dispatch-repository"]; !exists {
if configData, exists = outputMap["dispatch_repository"]; !exists {
return nil
}
// underscore form is deprecated; signal migration
dispatchRepositoryLog.Print("WARNING: safe-outputs.dispatch_repository is deprecated; rename to dispatch-repository or run ghawfix")
}This gives users actionable guidance without being a hard error.
@copilot please address this.
| "dispatch_repository": { | ||
| "$ref": "#/properties/safe-outputs/properties/dispatch-repository", | ||
| "description": "Deprecated alias for dispatch-repository." | ||
| }, |
There was a problem hiding this comment.
[/improve-codebase-architecture] The deprecated alias has a human-readable "description": "Deprecated alias for dispatch-repository." which is good, but JSON Schema draft 2019-09 and later support the "deprecated": true annotation for IDE/tooling signalling. Other deprecated aliases in this codebase do not use it either, but if tooling like Intellij or VS Code JSON language server honours it, users would get inline deprecation hints.
💡 Optional improvement
"dispatch_repository": {
"$ref": "#/properties/safe-outputs/properties/dispatch-repository",
"description": "Deprecated alias for dispatch-repository.",
"deprecated": true
},Low priority — only consider if your JSON schema tooling chain supports it.
@copilot please address this.
| assert.False(t, applied) | ||
| assert.Equal(t, content, result) | ||
| }) | ||
| } |
There was a problem hiding this comment.
[/tdd] The four test cases are a solid set, but there is a gap: the codemod skips migration when safe-outputs is absent from the frontmatter (handled by the early return), but there is no test for a file that has safe-outputs but dispatch_repository is not a direct child — e.g. it appears nested deeper or as a plain text value inside a description field. A false-positive rename in YAML value content would be a silent data corruption.
💡 Suggested additional test
t.Run("no-op when dispatch_repository appears only in a description value", func(t *testing.T) {
content := `---
safe-outputs:
some-tool:
description: "Triggers via dispatch_repository: mechanism"
workflow: router.yml
event_type: dispatch
repository: github/gh-aw
---
`
frontmatter := map[string]any{
"safe-outputs": map[string]any{
"some-tool": map[string]any{
"description": "Triggers via dispatch_repository: mechanism",
},
},
}
result, applied, err := codemod.Apply(content, frontmatter)
require.NoError(t, err)
assert.False(t, applied)
assert.Equal(t, content, result)
})@copilot please address this.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics & Test Classification (10 tests analyzed)
Go: 10 (
|
There was a problem hiding this comment.
REQUEST_CHANGES — Two blocking issues plus one medium concern.
🔴 Blocking: PR title and description are inverted relative to the implementation
The PR commits went through a mid-flight reversal (commit e35808a6a flipped underscore-first → dashed-first) but the title and description were never updated. The title still reads "remove dashed alias" while the code makes dispatch-repository the canonical key. Anyone using this PR as a reference for the change's intent — in blame, changelogs, or security audits — will reach the wrong conclusion. Please update the title/description to accurately state that dashed form is now canonical and underscore is the deprecated alias.
🟡 Blocking: Seven log messages in `dispatch_repository.go` still use the old `dispatch_repository` identifier
The entry log was updated, but the error paths and generateDispatchRepositoryTool were not touched. The result is a parse session that starts with "Parsing dispatch-repository configuration" and then reports "dispatch_repository value is not a map, skipping" — confusing for any operator who reads the logs. These belong in this PR since it is the change that made the naming inconsistent.
🟡 Non-blocking: silent skip when both keys coexist in a workflow
The codemod correctly avoids clobbering configs that have both dispatch_repository and dispatch-repository, but it emits no signal — not even a debug log — to tell the user their file was skipped and why. Consider adding a log line so gh aw fix is transparent about what it skipped.
🔎 Code quality review by PR Code Quality Reviewer · 102.9 AIC · ⌖ 7.45 AIC · ⊞ 5.2K
Comment /review to run again
| if configData, exists = outputMap["dispatch_repository"]; !exists { | ||
| if configData, exists = outputMap["dispatch-repository"]; !exists { | ||
| // dispatch-repository is canonical; keep underscore form as a backward-compatible alias. | ||
| if configData, exists = outputMap["dispatch-repository"]; !exists { |
There was a problem hiding this comment.
PR description is the inverse of the actual implementation: The PR title says "remove dashed alias" and the body shows dispatch_repository as the only accepted key after the change — but this code makes dispatch-repository (dashed) canonical and keeps dispatch_repository (underscore) as the backward-compatible fallback.
💡 Details and impact
The PR description reads:
// after: underscore-only
if configData, exists = outputMap["dispatch_repository"]; !exists {
return nil
}But the actual code shipped here is:
// dispatch-repository is canonical; keep underscore form as a backward-compatible alias.
if configData, exists = outputMap["dispatch-repository"]; !exists {
if configData, exists = outputMap["dispatch_repository"]; !exists {
return nil
}
}The implementation was reversed mid-PR (commit e35808a6a flipped from underscore-first to dashed-first), but the PR title and description were never updated. Any future maintainer reading "remove dashed alias" will confidently (and incorrectly) conclude that dispatch-repository is no longer accepted — when in fact it is now the primary key. The PR description must be corrected before merge.
| if configData, exists = outputMap["dispatch_repository"]; !exists { | ||
| if configData, exists = outputMap["dispatch-repository"]; !exists { | ||
| // dispatch-repository is canonical; keep underscore form as a backward-compatible alias. | ||
| if configData, exists = outputMap["dispatch-repository"]; !exists { |
There was a problem hiding this comment.
Stale log messages throughout the function still use the underscore form: The entry log and comment were updated to dispatch-repository, but the rest of parseDispatchRepositoryConfig and all of generateDispatchRepositoryTool still log dispatch_repository.
💡 Affected lines
- Line 45:
"dispatch_repository value is not a map, skipping" - Line 49:
"Parsing dispatch_repository tools map with %d entries" - Line 111:
"Parsed dispatch_repository tool %q: workflow=..." - Line 118:
"No valid tools found in dispatch_repository config" - Line 125: function comment
"generates an MCP tool definition for a specific dispatch_repository tool" - Line 129:
"Generating dispatch_repository tool: key=%s" - Line 168:
"Generated dispatch_repository tool: name=%s, properties=%d"
When a user configures safe-outputs.dispatch-repository and hits an error, the log says dispatch_repository value is not a map — a key they didn't use. This makes operational debugging actively misleading.
| } | ||
| _, hasOld := safeOutputsMap["dispatch_repository"] | ||
| _, hasNew := safeOutputsMap["dispatch-repository"] | ||
| return hasOld && !hasNew |
There was a problem hiding this comment.
Silent no-op when both keys coexist — no user-facing signal: When a workflow has both dispatch_repository and dispatch-repository under safe-outputs, gh aw fix will silently do nothing, leaving the user with a deprecated key that the parser ignores (since dispatch-repository takes priority at runtime).
💡 Suggested fix
At minimum emit a log that the user can see via --log-level debug (or upgrade to a warning), so they know manual cleanup is needed:
_, hasOld := safeOutputsMap["dispatch_repository"]
_, hasNew := safeOutputsMap["dispatch-repository"]
if hasOld && hasNew {
// Both keys present; skip migration but warn
safeOutputDispatchRepositoryKeyCodemodLog.Print(
"WARN: safe-outputs has both dispatch_repository and dispatch-repository; manual review needed")
return false
}
return hasOldWithout this, gh aw fix is not idempotent in user-perceptible terms: running it on a dual-key file produces no output and no change, but the deprecated key persists.
…decision Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (273 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
Review Summary
The code implementation is internally consistent — dispatch-repository (dashed) is correctly established as canonical throughout the compiler, schema, warning messages, docs, and tests. The codemod correctly migrates the underscore form. The correctness logic is sound.
However, there are three items worth addressing before merge:
⚠️ Critical: PR title and description contradict the implementation
The PR title says "remove dashed alias" and the body says "making underscore-only keying canonical," but the code does the exact opposite — it promotes dispatch-repository (dashed) to canonical and demotes dispatch_repository (underscore) to a deprecated alias with a migration codemod. All other safe-output keys in the project already use dashes (dispatch-workflow, call-workflow, merge-pull-request, etc.), so the dashed form is the correct convention. The code is right; the description needs to be rewritten to reflect what actually landed.
Suggested corrected description:
dispatch-repositoryis now the canonical key (matching the dashed convention of all other safe-output types)dispatch_repository(underscore) remains supported as a deprecated runtime alias and schema alias during the migration window- A codemod (
safe-output-dispatch-repository-key) migrates existing workflow files from underscore to dashed automatically - Schema, compiler warning, docs, and tests are all updated to the dashed form
🔧 Non-blocking: Stale log messages in dispatch_repository.go
Several log messages inside the same file still reference dispatch_repository (underscore) on lines 45, 49, 111, 118, 125, 129, 164, 168 — see inline comment. Minor but breaks the consistency signal introduced by this PR.
🔧 Non-blocking: IntroducedIn: "1.0.0" version accuracy
See inline comment on the codemod file. Recent codemods use the specific release version for traceability.
Files reviewed
pkg/workflow/dispatch_repository.go— lookup order swap ✅pkg/workflow/compiler_validators.go— warning message ✅pkg/workflow/dispatch_repository_test.go— test direction ✅pkg/workflow/dispatch_repository_experimental_warning_test.go— warning text ✅pkg/parser/schemas/main_workflow_schema.json— schema rename + alias ✅ (see inline note)pkg/parser/schema_location_test.go— positive acceptance test ✅pkg/parser/schema_test.go— schema path update ✅pkg/cli/codemod_safe_output_dispatch_repository_key.go— new codemod ✅ (see inline notes)pkg/cli/codemod_safe_output_dispatch_repository_key_test.go— tests ✅pkg/cli/fix_codemods.go— registration ✅pkg/cli/fix_codemods_test.go— expected order ✅docs/src/content/docs/reference/glossary.md— doc update ✅docs/src/content/docs/reference/safe-outputs.md— doc update ✅
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 68.9 AIC · ⌖ 9.82 AIC · ⊞ 4.9K
Comments that could not be inline-anchored
pkg/workflow/dispatch_repository.go:45
These log messages still use the underscore form while the function comment and lookup order were updated to the dashed canonical form. Lines 45 and 49 (and further at 111, 118, 125, 129, 164, 168) should be updated for consistency:
// line 45 - stale
dispatchRepositoryLog.Print("dispatch_repository value is not a map, skipping")
// line 49 - stale
dispatchRepositoryLog.Printf("Parsing dispatch_repository tools map with %d entries", ...)Should read dispatch-repository to match th…
pkg/cli/codemod_safe_output_dispatch_repository_key.go:16
The IntroducedIn version is "1.0.0" but newer codemods use the specific version in which the breaking change was made (e.g., "1.0.44", "1.0.47"). Since dispatch-repository is being made canonical in this release (v1.0.65+), this field should reflect the actual release version rather than 1.0.0. Using 1.0.0 is not harmful (the codemod is idempotent), but it makes the provenance harder to trace in the audit trail.
@copilot please address this.
pkg/parser/schemas/main_workflow_schema.json:9295
The dispatch_repository schema property is kept as a $ref alias to dispatch-repository. This means the schema still validates workflows using the underscore form — which is the right call for backward compatibility while the codemod migrates users.
However, the PR description says "Added parser validation coverage to ensure safe-outputs.dispatch-repository fails schema/location validation" — that description is incorrect on two counts:
- The added test in
schema_location_test.go…
|
@copilot run pr-finisher skill |
pelikhan
changed the title
dispatch_repository safe-output key (remove dashed alias)dispatch-repository safe-output key (deprecate underscore alias)
…mod improvements Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in the latest commit. Changes made:
|
|
🎉 This pull request is included in a new release. Release: |
dispatch-repositoryis now the canonical safe-output key, matching the dashed naming convention used by all other safe-output types (dispatch-workflow,call-workflow,merge-pull-request, etc.).dispatch_repository(underscore) remains supported as a deprecated runtime alias and schema alias during the migration window.Compiler behavior
safe-outputs.dispatch-repository(dashed) is now the canonical key, checked first.safe-outputs.dispatch_repository(underscore) remains a backward-compatible alias; a deprecation warning is logged when it is used.Codemod
gh aw fixincludes a newsafe-output-dispatch-repository-keycodemod that migratessafe-outputs.dispatch_repository→safe-outputs.dispatch-repositoryautomatically.Workflow parsing coverage
Schema
dispatch-repositoryis the primary schema property.dispatch_repositoryis kept as a$refalias for backward compatibility.